harden changeset dependency detection#1239
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| def emit(data): | ||
| """Write the result as JSON and exit successfully.""" | ||
| json.dump(data, sys.stdout) | ||
| sys.exit(0) |
There was a problem hiding this comment.
curiously, Is exiting with status 0 even on failures intentional?
There was a problem hiding this comment.
yeah, right now the script is treated as being run successfully as the consumer (CI job) gets additional failure metadata via emit.
I can restructure this to exit with a different code on failures if you feel strongly about it?
| direct = set() | ||
| for f in changed_files: | ||
| for pkg_name, pkg_dir in sorted_pkgs: | ||
| if f.startswith(pkg_dir + "/"): |
There was a problem hiding this comment.
will the script run on windows?
There was a problem hiding this comment.
unlikely, currently the intention for this script is to be run in CI only, but happy to add windows support if you think that's useful
There was a problem hiding this comment.
I don't see a case where we would need to run this for Windows in CI.
| def emit(data): | ||
| """Write the result as JSON and exit successfully.""" | ||
| json.dump(data, sys.stdout) | ||
| sys.exit(0) |
There was a problem hiding this comment.
I wonder if the sys.exit() should be called on the upper level rather than emit() function
There was a problem hiding this comment.
emit is the last step in the script, basically it's emit_and_exit, I can rename to make the intention clearer?
|
@xianshijing-lk I'll merge this for now, if you have any follow ups on the comments above, I'll address them in a separate PR |
3 participants
Before you submit your PR
Make sure the following is true before submitting your PR:
PR description
Describe the changes in this PR. Explain what the PR is meant to solve and how to reproduce the issue in the first place.
Breaking changes
If this PR introduces breaking changes, list them here and document the rationale for introducing such a change.
MSRV
If the PR modifies the crate's MSRV (Minimum Supported Rust Version), document it here.
Testing
Ideally, unit test the code you add, but ensure you're not repeating existing test cases. Use as many already written scaffolding, utilities as possible; write your own, when needed. If external services, APIs, tokens are required (e.g., running an LK server instance), provide the necessary information. Make sure your tests perform useful, context-aware assertions and do not simply emulate "happy paths".
Async
We want the project to be runtime-agnostic, so please reuse what's already in livekit-runtime and feel free to add anything missing. It's ok to use Tokio directly, when writing unit tests, if necessary. When testing, do not use artificial delays for the state to "catch up"; instead, respect the event flow and subscribe properly using channels or other mechanisms.